Updating npm #5812
Closed
Updating npm #5812
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Our frontend tests seem to be green, but there are some errors with log4js. One problem is that the tests are green although such errors happen. But the origin of the bug is In the future plugins cannot request modules like this, but should use |
|
Does anybody use Windows and can adapt bin/installOnWindows.bat? I failed badly... Most importantly is probably to check if NODE_ENV==production and change the npm link invocation accordingly. |
|
After the change, we will no longer have any problems with hoisting, as everything is in I checked which public plugins need to be adapted, see them below. Unfortunately this is a large amount, so I'll add a backwards compatible fix.
|
|
The async-stacktrace dependency was already removed in ebe05f8, so those plugins probably don't work anyway. When it comes to the other dependencies, I added symlinks in src/node_modules so the plugins should not break if we update and we can add PRs to those plugins. |
|
Wow! This is amazing, great job! We have software that can automatically resolve/remedy plugin issues provided by the foundation. https://github.com/ether/etherpad-lite/blob/develop/src/bin/plugins/checkPlugin.js -- I'd recommend using this. Would this break plugins on older versions of Etherpad tho? If so we should set the dependency within the plugin to depend on a newer version of Etherpad. |
webzwo0i
commented
Jul 6, 2023
src/package.json
Outdated
| @@ -36,6 +36,7 @@ | |||
| "cookie-parser": "^1.4.6", | |||
| "cross-spawn": "^7.0.3", | |||
| "ejs": "^3.1.9", | |||
| "ep_etherpad-lite": "file:", | |||
There was a problem hiding this comment.
Why add this here? All the files in ./src are actually "ep_etherpad-lite" (and ./src/package.json is the package.json that describes this module), so doesn't this introduce some kind of self-referenced loop?
There was a problem hiding this comment.
True. I don't know how this was added. Maybe by npm link?
There was a problem hiding this comment.
I'll continue with testing on Windows today.
|
Note to self: |
It works if I run w/ sudo. By works I mean I can access the new pad page but if I try to create a new pad I get an error -_- Not sure how tests are passing! |
It works on my end. All tests are passing so far on Linux. |
|
The tests fail because the complete return of the express server is undefined. This is only on Windows. I have no clue why. |
|
Thanks for testing. I see similar permission problems when building the Dockerfile. Will investigate. |
|
@JohnMcLear The permission problem is due to It affects all other projects that use npm under the same user, but I think we could add the above line to installDeps.sh, or maybe something like: |
OK, I can add a hint in checkPlugin.js. Plugins won't break, because I add symlinks for those dependencies that I could find (cheerio, express, formidable etc) that were in use by plugins. So |
|
Regarding The upgrade-from-last-release workflow also works now. It has some changes that need to be reverted after releasing. |
|
cc @webzwo0i |
|
I can nuke my I nuked node_modules and retried and it works fine other than error when trying to get to a pad |
That should be fixed with https://github.com/ether/etherpad-lite/pull/5812/files#diff-51658006a7d10c89a5d654c15b55a916dd3c4e1a1bd2462632f0116884e5f47eR166 Can you try reloading with ctrl+shift+r? |
|
I forgot I'd enabled cache so I went ahead and disabled it.. [2023-07-07 16:43:19.243] [INFO] plugins - npm --version: 9.6.7 I get this error still: I set maxAge to 0 and minify to false but still get this error. |
That should be fixed with https://github.com/ether/etherpad-lite/pull/5812/files#diff-51658006a7d10c89a5d654c15b55a916dd3c4e1a1bd2462632f0116884e5f47eR166 Can you try reloading with ctrl+shift+r?
The stack trace above indicates minify is still true?! Can you try with open developer console->network tab->"disable cache"? Although this is still a bad sign even if it would work, because most clients won't be so tech savvy and try all of this... :-( |
|
Good job so far. The startup is now insane fast because it doesn't start to redownload anything. I noticed there is a package.json file in the root of the project when running ./src/bin/run.sh maybe that should be included in the gitignore. |
|
@webzwo0i I'm 100% minify is set to false and I have "Disable Cache" checked.
I'm defo hitting the right instance too as if I kill it I get failure to connect. |
|
Is it really correct to remove all package.json? |
Weird. I don't get that problem on my Mac. Did you try reinstalling the project? I'm using v20.3.1 with 9.8.0. I have set minify to true, maxAge to 21600. It also works with minify: false and maxAge: 0 |
|
I don't know why but ep_etherpad_lite version 1.8.14 is in the package-lock.json bringing in npm version 6.14.13 but I can't find where this version 1.8.14 is coming from. |
|
Found it: https://www.npmjs.com/package/ep_etherpad-lite?activeTab=readme How do we update that? |
We use this package when testing rate limiting. We already install it in Docker, when running the Github workflow, so there is no need to install it by default. In contrast to other devDependencies this is not required in case you want to run the backend tests or check the code with eslint etc.
In some cases, when the server is restarting and the beforeEach hook tries to reload the admin/plugins page, it could fail.
SamTV12345
force-pushed
the
fix-admintests
branch
from
2517cf6
to
d2ebb06
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After trying to fix the failing admin tests for quite some time it is clear now that the problem is due to npm.
We have a number of bug reports (e.g. #5643 #5624 #5569 )
I even diagnosed this last year, but totally forgot about it. :-( #5609
During tests I noticed that in recent versions of npm packages again get deleted, if they are not stated in package.json. Because I don't have much insight into npm to solve all the problems we currently have, my approach is:
The two package.json files are different. The one in src only contains the dependencies of Etherpad core, while the file in the root directory essentially describes the whole project (core + plugins).
Admin tests, frontend tests and backend tests on linux already work.
We no longer need to re-install core with ./bin/installDeps.sh after installing plugins. We got some bad feedback because this behavior was counterintuitive.
As a result, only ROOT_DIR/node_modules contains packages anymore. src/node_modules is gone.
TODO:
@JohnMcLear @SamTV12345 What do you think? This is not 100% ready, but the idea seems to work in general.